-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Issue #2 - created my personal page ~ STYLES (Kartik) #27
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot, @ishtails! I love your design, really beautiful and cool implemented! :)
Below I left you some comments.
@@ -4,6 +4,335 @@ | |||
*/ | |||
import { GenericContractsDeclaration } from "~~/utils/scaffold-eth/contract"; | |||
|
|||
const deployedContracts = {} as const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would only push the file if it's a contract on real network, not a localhost one. It's useful for contributors when the contract is live, as they can interact with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey!! i am actually confused about this file. does changing the chain to optimism and redeploying fixes the issue? it seems like the file was auto-generated when i was testing on localhost. could you help me understand it better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once you deploy a contract to a chain, it is automatically created. This is useful when deploying a new contract to the mainnet, as it allows other contributors to know which external contracts are being used, along with their respective addresses. On a local network, however, the contract addresses can vary since everyone deploys their own instances. Since we're focusing on front-end work and not working on new smart contracts here, we'd prefer not to modify the file.
Hopefully that makes sense, pls let me know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alright! i got it. made changes to undo the modifications to this file. will keep this in mind next time. thanks!
packages/nextjs/scaffold.config.ts
Outdated
@@ -10,7 +10,7 @@ export type ScaffoldConfig = { | |||
|
|||
const scaffoldConfig = { | |||
// The networks on which your DApp is live | |||
targetNetworks: [chains.optimism], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the project is on Optimism, I wouldn't switch it to Hardhat.
}, | ||
]; | ||
|
||
const STYLESProfilePage = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might want to consider typing your page as NextPage
since we are using TypeScript, as this ensures better integration and adds clarity for future maintainers by indicating that the component is a Next.js page.
packages/nextjs/next.config.js
Outdated
@@ -14,6 +14,9 @@ const nextConfig = { | |||
config.externals.push("pino-pretty", "lokijs", "encoding"); | |||
return config; | |||
}, | |||
images: { | |||
domains: ["assets.aceternity.com"] | |||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if you need that. I couldn't find any external images in your codebase that comes from this domain, so you might want to consider removing it.
@@ -0,0 +1,73 @@ | |||
"use client"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might want to remove "use client;" here and apply it directly to
floating_bar.tsx```.
@@ -0,0 +1,73 @@ | |||
"use client"; | |||
|
|||
import React from "react"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to explicitly import React in Next.js anymore, starting from Next.js 12.2 and React 17 and above. With these versions, Next.js automatically imports React when needed, so you can safely omit import React from "react" at the top of your files.
@@ -0,0 +1,143 @@ | |||
"use client"; | |||
|
|||
import React from "react"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed
hey @phipsae!! thankyou for taking the time out to pin point these issues. i have made changes according to your suggestions, please review. |
and merged, thanks for the nice page! :) |
thank you! :)) |
Description
Added a new personal profile page at /builders/0x5D56b71abE6cA1Dc208Ed85926178f9758fa879c.
Additional Information
Screenshots:
Related Issues
_Closes #9 (Personal Page)
Your ENS/address: 0x5D56b71abE6cA1Dc208Ed85926178f9758fa879c